-
Notifications
You must be signed in to change notification settings - Fork 11
chore: version jobs go endpoint #689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR implements release-job linkage by removing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB
participant Store
Client->>Server: GetDeploymentVersionJobsList
Server->>DB: Load Deployment & Version
Server->>Store: Fetch System Environments
Server->>DB: Get Release Targets for Deployment
loop For each Environment
Server->>DB: Get Release Target Jobs
Server->>Server: Sort by Comparator (Failure > Status > CreatedAt > Name)
Server->>Server: Build fullReleaseTarget
end
Server->>Server: Sort Environments by Name
Server-->>Client: HTTP 200 with sorted targets & jobs
sequenceDiagram
participant Job Creator
participant writeJob
participant Store
participant DB (Release Check)
participant DB (Job Upsert)
participant writeReleaseJob
Job Creator->>writeJob: job with ReleaseId, store, tx
writeJob->>Store: Check Release exists
alt Release Not Found
Store-->>writeJob: Error
writeJob-->>Job Creator: Error
else Release Found
writeJob->>DB (Job Upsert): UPSERT job
DB (Job Upsert)-->>writeJob: Success
alt ReleaseId is non-empty
writeJob->>writeReleaseJob: releaseId, jobId, tx
writeReleaseJob->>DB (Release Check): INSERT release_job link
DB (Release Check)-->>writeReleaseJob: Success
writeReleaseJob-->>writeJob: Success
end
writeJob-->>Job Creator: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes The changes span multiple interconnected layers (database, server, SDK) with targeted logic changes. Core complexity areas include the comparator sorting rules, endpoint assembly logic, and store parameter threading through the changeset pipeline. While the scope is broad, individual changes are relatively straightforward transformations and lookups without dense algorithmic logic. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Code Coverage Reportworkspace-engine coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🧹 Nitpick comments (6)
packages/api/src/router/deployment-version-jobs-list.ts (3)
11-22: Prefer async/await for readability; avoid mixed .then chains.Keeps style consistent across the codebase and simplifies error stacks.
-const getWorkspaceId = async (tx: Tx, versionId: string) => - tx - .select() - .from(SCHEMA.deploymentVersion) - .innerJoin( - SCHEMA.deployment, - eq(SCHEMA.deploymentVersion.deploymentId, SCHEMA.deployment.id), - ) - .innerJoin(SCHEMA.system, eq(SCHEMA.deployment.systemId, SCHEMA.system.id)) - .where(eq(SCHEMA.deploymentVersion.id, versionId)) - .then(takeFirst) - .then(({ system }) => system.workspaceId); +const getWorkspaceId = async (tx: Tx, versionId: string) => { + const rows = await tx + .select() + .from(SCHEMA.deploymentVersion) + .innerJoin( + SCHEMA.deployment, + eq(SCHEMA.deploymentVersion.deploymentId, SCHEMA.deployment.id), + ) + .innerJoin(SCHEMA.system, eq(SCHEMA.deployment.systemId, SCHEMA.system.id)) + .where(eq(SCHEMA.deploymentVersion.id, versionId)); + const { system } = takeFirst(rows); + return system.workspaceId; +};
54-54: Harden error propagation from client.If the client sets
resp.errorwithout a nested.errorstring, the throw won’t trigger. Prefer checkingresp.errordirectly and/or includingresp.response?.status.- if (resp.error?.error != null) throw new Error(resp.error.error); + if (resp.error) { + const msg = (resp.error as any)?.error ?? "Workspace engine request failed"; + const status = (resp as any)?.response?.status; + throw new Error(status ? `${status}: ${msg}` : msg); + }
67-67: Avoid returning undefined; default to empty list.Downstream callers likely expect an array.
- return resp.data; + return resp.data ?? [];apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.go (1)
38-41: Confirm intended status ordering and nil-safety on Resource.
- Lexicographic status ordering (after failure precedence) may be surprising; confirm this is the desired UX.
a.Resource/b.Resourceare assumed non‑nil; guard or fallback to IDs to avoid panics if absent.Also applies to: 58-60
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go (1)
66-66: Consider replacing fixed sleeps with polling or event-driven synchronization.Hard-coded sleep statements make tests slower and can be flaky if processing takes longer than expected. Consider using a polling mechanism or waiting for specific events/conditions instead.
Also applies to: 120-120, 169-169, 329-329
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (1)
77-91: Consider logging when resource is not found.At line 84, when a resource is not found, the error is returned but this might indicate a data consistency issue (release target pointing to non-existent resource). Consider adding a log statement to help diagnose such issues.
func getFullReleaseTarget(ws *workspace.Workspace, releaseTarget *oapi.ReleaseTarget) (*fullReleaseTarget, error) { jobs, err := getReleaseTargetJobs(ws, releaseTarget) if err != nil { return nil, err } resource, ok := ws.Resources().Get(releaseTarget.ResourceId) if !ok { + // Log data consistency issue return nil, fmt.Errorf("resource %s not found", releaseTarget.ResourceId) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/workspace-engine/oapi/openapi.json(0 hunks)apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet(0 hunks)apps/workspace-engine/pkg/db/jobs.go(2 hunks)apps/workspace-engine/pkg/db/jobs_test.go(3 hunks)apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.go(1 hunks)apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator_test.go(1 hunks)apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go(1 hunks)apps/workspace-engine/pkg/workspace/populate_workspace.go(1 hunks)apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go(1 hunks)packages/api/src/router/deployment-version-jobs-list.ts(2 hunks)packages/workspace-engine-sdk/src/schema.ts(11 hunks)
💤 Files with no reviewable changes (2)
- apps/workspace-engine/oapi/openapi.json
- apps/workspace-engine/oapi/spec/paths/deployment-version.jsonnet
🧰 Additional context used
📓 Path-based instructions (4)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator_test.goapps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.goapps/workspace-engine/pkg/db/jobs.goapps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/db/jobs_test.goapps/workspace-engine/pkg/server/openapi/deploymentversions/server.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go
apps/workspace-engine/**/*_test.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
Follow the existing test structure used in *_test.go files
Files:
apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator_test.goapps/workspace-engine/pkg/db/jobs_test.goapps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
packages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
packages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
packages/workspace-engine-sdk/src/schema.tspackages/api/src/router/deployment-version-jobs-list.ts
🧠 Learnings (2)
📚 Learning: 2024-10-29T02:04:50.312Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/api/src/router/deployment.ts:116-131
Timestamp: 2024-10-29T02:04:50.312Z
Learning: In `packages/api/src/router/deployment.ts`, the `list.byDeploymentId` procedure requires multiple database queries due to limitations of the `releaseMatchesCondition` function.
Applied to files:
packages/api/src/router/deployment-version-jobs-list.ts
📚 Learning: 2025-08-12T18:13:54.630Z
Learnt from: CR
PR: ctrlplanedev/ctrlplane#0
File: apps/workspace-engine/CLAUDE.md:0-0
Timestamp: 2025-08-12T18:13:54.630Z
Learning: Applies to apps/workspace-engine/**/*_test.go : Follow the existing test structure used in *_test.go files
Applied to files:
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go
🧬 Code graph analysis (8)
apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (9)
Pending(41-41)Failure(37-37)Successful(43-43)InProgress(38-38)ActionRequired(34-34)ExternalRunNotFound(36-36)InvalidIntegration(39-39)InvalidJobAgent(40-40)Skipped(42-42)
apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (1)
Failure(37-37)
apps/workspace-engine/pkg/workspace/populate_workspace.go (1)
apps/workspace-engine/pkg/workspace/store/jobs.go (1)
Jobs(16-18)
packages/workspace-engine-sdk/src/schema.ts (1)
packages/workspace-engine-sdk/src/index.ts (2)
operations(5-5)components(6-6)
apps/workspace-engine/pkg/db/jobs_test.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (3)
Pending(41-41)InProgress(38-38)Successful(43-43)
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (1)
apps/workspace-engine/pkg/oapi/oapi.gen.go (5)
Environment(153-160)ReleaseTarget(337-341)Job(183-194)Deployment(107-116)Resource(344-358)
packages/api/src/router/deployment-version-jobs-list.ts (2)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/api/src/workspace-engine-client.ts (1)
getWorkspaceEngineClient(9-12)
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go (2)
apps/workspace-engine/test/integration/opts.go (14)
WithJobAgent(134-150)JobAgentID(525-529)WithSystem(84-105)SystemName(241-245)WithDeployment(259-277)DeploymentID(310-314)DeploymentName(297-302)DeploymentJobAgent(316-320)WithDeploymentVersion(353-367)DeploymentVersionID(383-387)WithEnvironment(279-293)EnvironmentName(415-419)WithResource(107-132)ResourceName(449-453)apps/workspace-engine/pkg/oapi/oapi.gen.go (4)
Failure(37-37)InProgress(38-38)Successful(43-43)Pending(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: workspace-engine-tests
- GitHub Check: build (linux/amd64)
🔇 Additional comments (9)
packages/api/src/router/deployment-version-jobs-list.ts (2)
42-52: Remote call integration LGTM.Path params wiring and client usage look correct.
4-4: No issues found with the import statement.The import is correct. The
@ctrlplane/dbpackage properly re-exports botheqandtakeFirst:
eqis available through the barrel exportexport * from "drizzle-orm/pg-core"takeFirstis explicitly exported from./common.jsBoth symbols will resolve without compile errors.
packages/workspace-engine-sdk/src/schema.ts (1)
27-46: Generated SDK additions look coherent with server surface.New endpoints and
relatableEntityTypeparameter rename are consistent; no API surface concerns from types alone.Please confirm the SDK was regenerated from the same OpenAPI commit as the server changes to avoid drift.
Also applies to: 47-66, 87-106, 107-126, 147-166, 167-186, 247-266, 267-286, 287-306, 662-725, 726-768, 854-896, 933-966, 968-1009, 1121-1158, 1159-1201, 1202-1244, 631-633, 813-813
apps/workspace-engine/pkg/db/jobs.go (1)
199-209: Good: propagate linkage errors to caller.Error propagation after job upsert is correct and prevents silent data corruption.
apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator_test.go (1)
1-335: Excellent test coverage for the comparator.The test suite comprehensively covers all edge cases and comparison scenarios including nil jobs, failure priority, lexicographic ordering, timestamp-based sorting, and resource name tiebreaking. The use of helper functions and subtests makes the tests clear and maintainable.
apps/workspace-engine/pkg/db/jobs_test.go (2)
241-259: Good verification of automatic release_job association creation.The added verification ensures that the release_job association is automatically created when writing a job, which aligns with the new behavior described in the PR.
839-1020: Comprehensive test coverage for release_job associations.This test thoroughly validates the complete flow: writing multiple jobs, verifying automatic associations, retrieving jobs, confirming linkage, and ensuring associations persist after updates. The test structure is clear and follows the existing patterns.
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go (2)
16-62: LGTM! Clean helper functions.The helper functions are well-structured and follow single-responsibility principle. They appropriately filter and transform data for the endpoint.
145-147: No action required — the function is properly accessible.The
compareReleaseTargetsfunction is correctly defined incomparator.goas a package-private function (lowercase name), which is the appropriate access level for functions used internally within the same package. Bothcomparator.goandserver.goare in thedeploymentversionspackage, so the function call at lines 145-147 is valid and follows standard Go conventions.Likely an incorrect or invalid review comment.
| const RELEASE_JOB_CHECK_QUERY = ` | ||
| SELECT EXISTS(SELECT 1 FROM release_job WHERE release_id = $1 AND job_id = $2) | ||
| ` | ||
|
|
||
| const RELEASE_JOB_INSERT_QUERY = ` | ||
| INSERT INTO release_job (release_id, job_id) | ||
| VALUES ($1, $2) | ||
| ` | ||
|
|
||
| func writeReleaseJob(ctx context.Context, releaseId string, jobId string, tx pgx.Tx) error { | ||
| // Check if the association already exists | ||
| var exists bool | ||
| err := tx.QueryRow(ctx, RELEASE_JOB_CHECK_QUERY, releaseId, jobId).Scan(&exists) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Only insert if it doesn't exist | ||
| if !exists { | ||
| _, err := tx.Exec(ctx, RELEASE_JOB_INSERT_QUERY, releaseId, jobId) | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Eliminate check‑then‑insert race for release_job.
Concurrent writers can hit unique‑violation between EXISTS and INSERT. Use a single UPSERT/DO NOTHING.
-const RELEASE_JOB_CHECK_QUERY = `
- SELECT EXISTS(SELECT 1 FROM release_job WHERE release_id = $1 AND job_id = $2)
-`
-
-const RELEASE_JOB_INSERT_QUERY = `
- INSERT INTO release_job (release_id, job_id)
- VALUES ($1, $2)
-`
+const RELEASE_JOB_INSERT_QUERY = `
+ INSERT INTO release_job (release_id, job_id)
+ VALUES ($1, $2)
+ ON CONFLICT (release_id, job_id) DO NOTHING
+`
@@
-func writeReleaseJob(ctx context.Context, releaseId string, jobId string, tx pgx.Tx) error {
- // Check if the association already exists
- var exists bool
- err := tx.QueryRow(ctx, RELEASE_JOB_CHECK_QUERY, releaseId, jobId).Scan(&exists)
- if err != nil {
- return err
- }
-
- // Only insert if it doesn't exist
- if !exists {
- _, err := tx.Exec(ctx, RELEASE_JOB_INSERT_QUERY, releaseId, jobId)
- return err
- }
-
- return nil
-}
+func writeReleaseJob(ctx context.Context, releaseId string, jobId string, tx pgx.Tx) error {
+ _, err := tx.Exec(ctx, RELEASE_JOB_INSERT_QUERY, releaseId, jobId)
+ return err
+}Ensure a unique constraint exists on (release_id, job_id) in release_job to support the ON CONFLICT clause.
🏁 Script executed:
#!/bin/bash
# Find and examine the jobs.go file
fd -t f "jobs.go" apps/workspace-engineLength of output: 140
🏁 Script executed:
#!/bin/bash
# Examine the writeReleaseJob function and surrounding code
cat apps/workspace-engine/pkg/db/jobs.go | grep -A 30 "writeReleaseJob"Length of output: 1090
🏁 Script executed:
#!/bin/bash
# Search for database migrations or schema definitions for release_job
fd -t f "\.(sql|go)$" apps/workspace-engine | xargs grep -l "release_job" | head -20Length of output: 78
🏁 Script executed:
#!/bin/bash
# Look for table creation or migration files
fd -t f -e sql apps/workspace-engine | head -20Length of output: 389
🏁 Script executed:
#!/bin/bash
# Search for any constraint definitions on release_job
rg -n "release_job" apps/workspace-engine --type=sql --type=go -C 2Length of output: 6410
🏁 Script executed:
#!/bin/bash
# Check for any references to unique constraints or conflicts
rg -n "UNIQUE|ON CONFLICT|unique constraint" apps/workspace-engine/pkg/dbLength of output: 2479
🏁 Script executed:
#!/bin/bash
# Examine the full context of jobs.go around line 132-156
sed -n '120,170p' apps/workspace-engine/pkg/db/jobs.goLength of output: 1331
🏁 Script executed:
#!/bin/bash
# Look for any table schema or struct definitions related to release_job
rg -n "type.*Release" apps/workspace-engine/pkg/db -A 5 -B 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find migration files
fd -t f "migrations" apps/workspace-engine -x find {} -name "*.sql"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search broadly for release_job mentions
rg -n "release_job" apps/workspace-engineLength of output: 2254
🏁 Script executed:
#!/bin/bash
# Look at the imports and transaction patterns in jobs.go
head -50 apps/workspace-engine/pkg/db/jobs.goLength of output: 1005
🏁 Script executed:
#!/bin/bash
# Check if there are test files that show how writeReleaseJob is used
fd -t f "jobs_test.go\|jobs.*test" apps/workspace-engineLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any concurrent access patterns
rg -n "goroutine|concurrent|Parallel|go " apps/workspace-engine/pkg/db/jobs.goLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find database initialization or setup code
rg -n "CREATE TABLE.*release_job\|release_job.*FOREIGN KEY" apps/workspace-engineLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the entire jobs.go file for constants and queries
cat apps/workspace-engine/pkg/db/jobs.goLength of output: 4803
🏁 Script executed:
#!/bin/bash
# Look for postgres version or transaction isolation configuration
rg -n "isolation\|Serializable\|Repeatable\|transaction" apps/workspace-engine/pkg/dbLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for how writeReleaseJob is called
rg -n "writeReleaseJob" apps/workspace-engine -C 3Length of output: 957
🏁 Script executed:
#!/bin/bash
# Look for any existing upsert patterns in the codebase
rg -n "ON CONFLICT|DO NOTHING|DO UPDATE" apps/workspace-engineLength of output: 3368
🏁 Script executed:
#!/bin/bash
# Check for database schema files
fd -t f -e schema apps/workspace-engineLength of output: 389
🏁 Script executed:
#!/bin/bash
# Look for any constraint definitions in Go struct tags
rg -n "gorm|db:|sql:" apps/workspace-engine/pkg/db -A 1 -B 1 | grep -i "unique\|constraint"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find any references to the release_job table schema
rg -n "release_id.*job_id\|job_id.*release_id" apps/workspace-engineLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any error handling patterns for unique constraint violations
rg -n "unique.*violation\|constraint.*violation\|duplicate.*key" apps/workspace-engineLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find database initialization or migration runner
fd -t f "init\|setup\|migrate" apps/workspace-engine/pkg/dbLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the complete database package structure
find apps/workspace-engine/pkg/db -type f -name "*.go" | head -20Length of output: 951
🏁 Script executed:
#!/bin/bash
# Search for table definitions or database schema documentation
rg -n "TABLE\|schema\|Schema" apps/workspace-engine/pkg/db --max-count=30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how transactions are created and used
rg -n "pgx\.Tx\|tx\.Query\|tx\.Exec" apps/workspace-engine/pkg/db/jobs.go -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the caller of writeReleaseJob to understand concurrency context
rg -n "writeReleaseJob" apps/workspace-engine --type=go -B 10 -A 5Length of output: 2051
🏁 Script executed:
#!/bin/bash
# Check if there are any unique constraints already defined
find apps/workspace-engine -name "*.sql" -o -name "*.go" | xargs grep -l "PRIMARY KEY\|UNIQUE" 2>/dev/null | head -10Length of output: 88
🏁 Script executed:
#!/bin/bash
# List all SQL files in the workspace-engine
find apps/workspace-engine -name "*.sql" -type fLength of output: 0
🏁 Script executed:
#!/bin/bash
# Examine the structure of the code and any constraints
fd -t f -x wc -l {} \; apps/workspace-engine/pkg/dbLength of output: 417
🏁 Script executed:
#!/bin/bash
# Check the final content of jobs.go to confirm the current state
tail -100 apps/workspace-engine/pkg/db/jobs.goLength of output: 2294
🏁 Script executed:
#!/bin/bash
# Search in root level SQL files
find . -maxdepth 5 -name "*.sql" -type fLength of output: 6337
🏁 Script executed:
#!/bin/bash
# Look for schema or migrations directory
find apps -type d -name "*migrat*" -o -type d -name "*schema*" | head -10Length of output: 220
🏁 Script executed:
#!/bin/bash
# Get context around lines 132-156 in jobs.go with line numbers
sed -n '100,180p' apps/workspace-engine/pkg/db/jobs.go | cat -nLength of output: 2584
🏁 Script executed:
#!/bin/bash
# Check if the code currently has the ON CONFLICT clause or the old pattern
grep -n "ON CONFLICT\|RELEASE_JOB_CHECK_QUERY\|RELEASE_JOB_INSERT_QUERY" apps/workspace-engine/pkg/db/jobs.goLength of output: 271
🏁 Script executed:
#!/bin/bash
# Search drizzle migrations for release_job table
grep -r "release_job" packages/db/drizzle/ 2>/dev/null | head -20Length of output: 3081
🏁 Script executed:
#!/bin/bash
# Look for table creation in drizzle migrations
grep -r "CREATE TABLE.*release_job\|release_job" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 4624
🏁 Script executed:
#!/bin/bash
# Check test setup in jobs_test.go for table creation
grep -n "CREATE TABLE\|release_job" apps/workspace-engine/pkg/db/jobs_test.go | head -30Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Search for database schema initialization
find apps/workspace-engine -name "*.go" -type f -exec grep -l "CREATE TABLE" {} \;Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for any SQL schema files or schema definitions
find . -name "schema*.sql" -o -name "*schema.sql" 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a setup.go or db.go that initializes schema
grep -r "CREATE TABLE.*release_job\|release_job.*PRIMARY\|release_job.*UNIQUE" apps/workspace-engine --include="*.go" -A 3 -B 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at jobs_test.go for any setup that might reveal schema
head -100 apps/workspace-engine/pkg/db/jobs_test.goLength of output: 3100
🏁 Script executed:
#!/bin/bash
# Search for constraint or foreign key definitions
grep -n "PRIMARY KEY\|UNIQUE\|FOREIGN KEY" apps/workspace-engine/pkg/db/*.go | grep -i "release"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the drizzle 0000 migration for schema baseline
cat packages/db/drizzle/0000_busy_firebird.sql | grep -A 10 "release_job" 2>/dev/null || echo "Not in first migration"Length of output: 2814
🏁 Script executed:
#!/bin/bash
# Search all drizzle migrations for release_job
grep -l "release_job" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 518
🏁 Script executed:
#!/bin/bash
# Get the full content of drizzle migrations that mention release_job
for file in $(grep -l "release_job" packages/db/drizzle/*.sql 2>/dev/null); do echo "=== $file ==="; cat "$file" | grep -A 20 "release_job"; doneLength of output: 15473
🏁 Script executed:
#!/bin/bash
# Search for any unique constraint pattern in existing migrations
grep "UNIQUE.*release_id.*job_id\|UNIQUE.*job_id.*release_id" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# List the last few migration files to see if schema is recent
ls -lah packages/db/drizzle/ | tail -20Length of output: 1491
🏁 Script executed:
#!/bin/bash
# Check if there's a references or relationships file for foreign keys
cat packages/db/drizzle/0001_smooth_mandroid.sql 2>/dev/null | head -50Length of output: 171
🏁 Script executed:
#!/bin/bash
# Search for PRIMARY KEY on release_job in migrations
grep -n "release_job.*PRIMARY\|PRIMARY.*release_job" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any composite key or index on release_id, job_id
grep -n "release_id.*job_id\|job_id.*release_id" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the current test setup to see if it creates the table
grep -A 30 "setupTestWithWorkspace\|func setupTest" apps/workspace-engine/pkg/db/jobs_test.go | head -50Length of output: 1180
🏁 Script executed:
#!/bin/bash
# Look for database client initialization
cat apps/workspace-engine/pkg/db/client.go 2>/dev/null | head -50Length of output: 1140
🏁 Script executed:
#!/bin/bash
# Look for any schema or migrations reference in workspace-engine
find apps/workspace-engine -type f -name "*.go" -exec grep -l "migrate\|schema" {} \; | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if workspace-engine references packages/db
grep -r "packages/db\|packages/db/drizzle" apps/workspace-engine/ --include="*.go" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for composite index on release_id, job_id
grep "CREATE.*INDEX.*release_id.*job_id\|CREATE.*INDEX.*job_id.*release_id" packages/db/drizzle/*.sql 2>/dev/nullLength of output: 0
🏁 Script executed:
#!/bin/bash
# List migration files and search for the one that creates release_job
for f in packages/db/drizzle/*.sql; do if grep -q "release_job" "$f"; then echo "$f"; fi; doneLength of output: 518
🏁 Script executed:
#!/bin/bash
# Get the content of migration files containing release_job
for f in packages/db/drizzle/*.sql; do
if grep -q "release_job" "$f"; then
echo "=== $(basename $f) ===";
cat "$f";
echo "";
fi;
doneLength of output: 68331
🏁 Script executed:
#!/bin/bash
# Check if any migration has ON CONFLICT pattern
grep -l "ON CONFLICT" packages/db/drizzle/*.sql 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for test database setup that might show schema
grep -r "setupTest\|SetupTest" apps/workspace-engine/pkg/db/*.go -A 20 | grep -i "create\|table\|schema" | head -20Length of output: 1901
Add UNIQUE constraint migration before implementing ON CONFLICT code change.
The race condition is real: concurrent transactions can insert duplicate (release_id, job_id) pairs between the EXISTS check and INSERT. However, the proposed ON CONFLICT solution requires a unique constraint that does not currently exist in the schema.
Migration 0081 creates the release_job table without a UNIQUE constraint on (release_id, job_id). You must:
- Create a migration adding
UNIQUE (release_id, job_id)constraint to release_job - Then apply the code changes in jobs.go to use
ON CONFLICT (release_id, job_id) DO NOTHING
Without the schema change, the ON CONFLICT clause will not know which rows conflict and the code will fail.
| if createdAtA != createdAtB { | ||
| // Return negative if b is more recent (b should come first) | ||
| return int(createdAtB - createdAtA) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix misleading comment and avoid int cast for time diff.
Comment says “return negative if b is more recent” but the code returns positive in that case. Also, casting int64 diff to int is unnecessary and could overflow on 32‑bit builds.
- if createdAtA != createdAtB {
- // Return negative if b is more recent (b should come first)
- return int(createdAtB - createdAtA)
- }
+ if createdAtA != createdAtB {
+ // More recent first: return positive if b is newer, negative if a is newer.
+ if createdAtB > createdAtA {
+ return 1
+ }
+ return -1
+ }🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.go
around lines 53 to 56, the comment is reversed and the code casts an int64 time
difference to int which can overflow; update the comment to accurately state the
return convention (e.g. "return positive if b is more recent (b should come
first)"), and replace the int64 subtraction-and-cast with an explicit comparison
that returns 1 or -1 (or -1/1 consistent with the comparator contract) to avoid
casting from int64 to int and eliminate overflow risk.
| func (s *DeploymentVersions) GetDeploymentVersionJobsList(c *gin.Context, workspaceId string, versionId string) { | ||
| panic("not implemented") | ||
| ws, err := utils.GetWorkspace(c, workspaceId) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| version, ok := ws.DeploymentVersions().Get(versionId) | ||
| if !ok { | ||
| c.JSON(http.StatusNotFound, gin.H{ | ||
| "error": fmt.Errorf("deployment version %s not found", versionId).Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| deployment, ok := ws.Deployments().Get(version.DeploymentId) | ||
| if !ok { | ||
| c.JSON(http.StatusNotFound, gin.H{ | ||
| "error": fmt.Errorf("deployment %s not found", version.DeploymentId).Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| environments := getSystemEnvironments(ws, deployment.SystemId) | ||
| releaseTargets, err := getDeploymentReleaseTargets(c, ws, deployment.Id) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| envsWithTargets := make([]*environmentWithTargets, 0) | ||
|
|
||
| for _, environment := range environments { | ||
| environmentReleaseTargets := getEnvironmentReleaseTargets(releaseTargets, environment.Id) | ||
| fullReleaseTargets := make([]*fullReleaseTarget, 0) | ||
| for _, releaseTarget := range environmentReleaseTargets { | ||
| fullReleaseTarget, err := getFullReleaseTarget(ws, releaseTarget) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| return | ||
| } | ||
| fullReleaseTarget.Environment = environment | ||
| fullReleaseTarget.Deployment = deployment | ||
| fullReleaseTargets = append(fullReleaseTargets, fullReleaseTarget) | ||
| } | ||
|
|
||
| sort.Slice(fullReleaseTargets, func(i, j int) bool { | ||
| return compareReleaseTargets(fullReleaseTargets[i], fullReleaseTargets[j]) < 0 | ||
| }) | ||
| envWithTarget := &environmentWithTargets{ | ||
| Environment: environment, | ||
| ReleaseTargets: fullReleaseTargets, | ||
| } | ||
| envsWithTargets = append(envsWithTargets, envWithTarget) | ||
| } | ||
|
|
||
| sort.Slice(envsWithTargets, func(i, j int) bool { | ||
| return envsWithTargets[i].Environment.Name < envsWithTargets[j].Environment.Name | ||
| }) | ||
| c.JSON(http.StatusOK, envsWithTargets) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Add documentation for exported endpoint function.
The exported function GetDeploymentVersionJobsList lacks documentation explaining its purpose, parameters, and return behavior.
As per coding guidelines:
+// GetDeploymentVersionJobsList retrieves all jobs for a deployment version,
+// grouped by environment and sorted by status priority and timestamp.
+// Returns 404 if deployment version or deployment not found.
+// Returns 500 on internal errors.
func (s *DeploymentVersions) GetDeploymentVersionJobsList(c *gin.Context, workspaceId string, versionId string) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *DeploymentVersions) GetDeploymentVersionJobsList(c *gin.Context, workspaceId string, versionId string) { | |
| panic("not implemented") | |
| ws, err := utils.GetWorkspace(c, workspaceId) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| version, ok := ws.DeploymentVersions().Get(versionId) | |
| if !ok { | |
| c.JSON(http.StatusNotFound, gin.H{ | |
| "error": fmt.Errorf("deployment version %s not found", versionId).Error(), | |
| }) | |
| return | |
| } | |
| deployment, ok := ws.Deployments().Get(version.DeploymentId) | |
| if !ok { | |
| c.JSON(http.StatusNotFound, gin.H{ | |
| "error": fmt.Errorf("deployment %s not found", version.DeploymentId).Error(), | |
| }) | |
| return | |
| } | |
| environments := getSystemEnvironments(ws, deployment.SystemId) | |
| releaseTargets, err := getDeploymentReleaseTargets(c, ws, deployment.Id) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| envsWithTargets := make([]*environmentWithTargets, 0) | |
| for _, environment := range environments { | |
| environmentReleaseTargets := getEnvironmentReleaseTargets(releaseTargets, environment.Id) | |
| fullReleaseTargets := make([]*fullReleaseTarget, 0) | |
| for _, releaseTarget := range environmentReleaseTargets { | |
| fullReleaseTarget, err := getFullReleaseTarget(ws, releaseTarget) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| fullReleaseTarget.Environment = environment | |
| fullReleaseTarget.Deployment = deployment | |
| fullReleaseTargets = append(fullReleaseTargets, fullReleaseTarget) | |
| } | |
| sort.Slice(fullReleaseTargets, func(i, j int) bool { | |
| return compareReleaseTargets(fullReleaseTargets[i], fullReleaseTargets[j]) < 0 | |
| }) | |
| envWithTarget := &environmentWithTargets{ | |
| Environment: environment, | |
| ReleaseTargets: fullReleaseTargets, | |
| } | |
| envsWithTargets = append(envsWithTargets, envWithTarget) | |
| } | |
| sort.Slice(envsWithTargets, func(i, j int) bool { | |
| return envsWithTargets[i].Environment.Name < envsWithTargets[j].Environment.Name | |
| }) | |
| c.JSON(http.StatusOK, envsWithTargets) | |
| } | |
| // GetDeploymentVersionJobsList retrieves all jobs for a deployment version, | |
| // grouped by environment and sorted by status priority and timestamp. | |
| // Returns 404 if deployment version or deployment not found. | |
| // Returns 500 on internal errors. | |
| func (s *DeploymentVersions) GetDeploymentVersionJobsList(c *gin.Context, workspaceId string, versionId string) { | |
| ws, err := utils.GetWorkspace(c, workspaceId) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| version, ok := ws.DeploymentVersions().Get(versionId) | |
| if !ok { | |
| c.JSON(http.StatusNotFound, gin.H{ | |
| "error": fmt.Errorf("deployment version %s not found", versionId).Error(), | |
| }) | |
| return | |
| } | |
| deployment, ok := ws.Deployments().Get(version.DeploymentId) | |
| if !ok { | |
| c.JSON(http.StatusNotFound, gin.H{ | |
| "error": fmt.Errorf("deployment %s not found", version.DeploymentId).Error(), | |
| }) | |
| return | |
| } | |
| environments := getSystemEnvironments(ws, deployment.SystemId) | |
| releaseTargets, err := getDeploymentReleaseTargets(c, ws, deployment.Id) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| envsWithTargets := make([]*environmentWithTargets, 0) | |
| for _, environment := range environments { | |
| environmentReleaseTargets := getEnvironmentReleaseTargets(releaseTargets, environment.Id) | |
| fullReleaseTargets := make([]*fullReleaseTarget, 0) | |
| for _, releaseTarget := range environmentReleaseTargets { | |
| fullReleaseTarget, err := getFullReleaseTarget(ws, releaseTarget) | |
| if err != nil { | |
| c.JSON(http.StatusInternalServerError, gin.H{ | |
| "error": err.Error(), | |
| }) | |
| return | |
| } | |
| fullReleaseTarget.Environment = environment | |
| fullReleaseTarget.Deployment = deployment | |
| fullReleaseTargets = append(fullReleaseTargets, fullReleaseTarget) | |
| } | |
| sort.Slice(fullReleaseTargets, func(i, j int) bool { | |
| return compareReleaseTargets(fullReleaseTargets[i], fullReleaseTargets[j]) < 0 | |
| }) | |
| envWithTarget := &environmentWithTargets{ | |
| Environment: environment, | |
| ReleaseTargets: fullReleaseTargets, | |
| } | |
| envsWithTargets = append(envsWithTargets, envWithTarget) | |
| } | |
| sort.Slice(envsWithTargets, func(i, j int) bool { | |
| return envsWithTargets[i].Environment.Name < envsWithTargets[j].Environment.Name | |
| }) | |
| c.JSON(http.StatusOK, envsWithTargets) | |
| } |
🤖 Prompt for AI Agents
apps/workspace-engine/pkg/server/openapi/deploymentversions/server.go lines
93-159: the exported function GetDeploymentVersionJobsList is missing a Go doc
comment; add a descriptive comment immediately above the function declaration
that starts with the function name (GetDeploymentVersionJobsList) and briefly
explains its purpose, the meaning of its receiver and parameters (c
*gin.Context, workspaceId, versionId), expected behavior (what HTTP responses
are returned in success and error cases), and any side effects; keep it concise,
use proper Go doc convention so linters and generated docs pick it up.
| for _, job := range initialWorkspaceState.Jobs() { | ||
| ws.Jobs().Upsert(ctx, job) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not ignore errors when upserting Jobs.
Silent failures will leave workspace state inconsistent (and can hide release↔job linkage errors).
for _, job := range initialWorkspaceState.Jobs() {
- ws.Jobs().Upsert(ctx, job)
+ if err := ws.Jobs().Upsert(ctx, job); err != nil {
+ return err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, job := range initialWorkspaceState.Jobs() { | |
| ws.Jobs().Upsert(ctx, job) | |
| } | |
| for _, job := range initialWorkspaceState.Jobs() { | |
| if err := ws.Jobs().Upsert(ctx, job); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/workspace/populate_workspace.go around lines 50 to
52, the loop calling ws.Jobs().Upsert(ctx, job) ignores returned errors which
can hide failures; modify the loop to capture the error from Upsert for each
job, wrap or annotate it with context (e.g., which job ID or name) and return
that error (or otherwise propagate it) instead of discarding it so the caller
can handle/fail on upsert problems.
apps/workspace-engine/test/e2e/engine_deployment_version_jobs_list_test.go
Outdated
Show resolved
Hide resolved
| compareReleaseTargets := func(a, b *fullReleaseTarget) int { | ||
| var statusA *oapi.JobStatus | ||
| var statusB *oapi.JobStatus | ||
|
|
||
| if len(a.Jobs) > 0 { | ||
| statusA = &a.Jobs[0].Status | ||
| } | ||
| if len(b.Jobs) > 0 { | ||
| statusB = &b.Jobs[0].Status | ||
| } | ||
|
|
||
| if statusA == nil && statusB != nil { | ||
| return 1 | ||
| } | ||
| if statusA != nil && statusB == nil { | ||
| return -1 | ||
| } | ||
|
|
||
| if statusA != nil && statusB != nil { | ||
| if *statusA == oapi.Failure && *statusB != oapi.Failure { | ||
| return -1 | ||
| } | ||
| if *statusA != oapi.Failure && *statusB == oapi.Failure { | ||
| return 1 | ||
| } | ||
|
|
||
| if *statusA != *statusB { | ||
| if string(*statusA) < string(*statusB) { | ||
| return -1 | ||
| } | ||
| return 1 | ||
| } | ||
| } | ||
|
|
||
| var createdAtA, createdAtB int64 | ||
| if len(a.Jobs) > 0 { | ||
| createdAtA = a.Jobs[0].CreatedAt.Unix() | ||
| } | ||
| if len(b.Jobs) > 0 { | ||
| createdAtB = b.Jobs[0].CreatedAt.Unix() | ||
| } | ||
|
|
||
| if createdAtA != createdAtB { | ||
| return int(createdAtB - createdAtA) | ||
| } | ||
|
|
||
| if a.Resource.Name < b.Resource.Name { | ||
| return -1 | ||
| } else if a.Resource.Name > b.Resource.Name { | ||
| return 1 | ||
| } | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Eliminate duplicate comparator implementation.
The comparator logic is duplicated in two places (lines 229-281 and 525-577). This creates a maintenance burden and risks divergence from the actual implementation in comparator.go.
Consider either:
- Importing and using
compareReleaseTargetsfrom thedeploymentversionspackage (if you can export it for testing) - Creating a shared test helper package
- Keeping only one copy in a test helper function and reusing it
Since the actual implementation is in apps/workspace-engine/pkg/server/openapi/deploymentversions/comparator.go, you could export it or create a test-only export for validation.
Also applies to: 525-577
📊 Code Coverage Reportworkspace-engine coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
📊 DB Package Test Coveragepkg/db coverage: |
📊 Code Coverage Reportworkspace-engine coverage: |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
rolloutTimefield from release target objects.